Repository navigation
Compile character range patterns in linear time - #8868
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8868 +/- ##
==========================================
+ Coverage 80.99% 81.02% +0.02%
==========================================
Files 453 453
Lines 62989 63030 +41
==========================================
+ Hits 51020 51070 +50
+ Misses 11969 11960 -9
🚀 New features to boost your workflow:
|
Skip the no-op alpha renaming of variable-free or-patterns, and find pattern groups, default matrix minima and switch cells by hashing. Fixes #8866. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KDBKAdTcAxA3HsjtVESRCr Signed-off-by: Christoph Knittel <ck@cca.io>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KDBKAdTcAxA3HsjtVESRCr Signed-off-by: Christoph Knittel <ck@cca.io>
d61ce0b to
4bb9898
Compare
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
|
Developer playground preview: https://rescript-lang.github.io/rescript/dev-playground/?version=pr-8868 |
cristianoc
left a comment
There was a problem hiding this comment.
TIL this was not done in linear time earlier
#8868 made exhaustiveness checking and match compilation find constants by hashing, because an expanded char range turned one pattern into thousands of distinct constants. Char ranges are now one pattern each, so the hash tables in `Parmatch.filter_all`, `Parmatch.get_mins_pats` and `Matching.divide`, and the `const_key` that kept their keys consistent with char/int equality, are removed. A dead `char` check in comparison specialization goes too: `char` expands to `int` before it. Review: How much slower do large switches compile? A switch with 1,000 int or string cases compiles 0.02-0.03 s slower; one with 10,000 int cases takes 6.3 s instead of 3.2 s. Polymorphic variants are unaffected: 10,000 cases take about 35 s either way, as other quadratic steps dominate. Review: Does the output change? No. All committed snapshots are byte-identical, and so is the JS of 1,500 fuzzer programs and the benchmark switches. Area: typechecker lambda Output: unchanged Formatting: unchanged Formats: none PPX: unchanged Editor: unchanged Tests: none, because the output is unchanged and existing tests cover the behaviour Measured: best of 3 bsc wall time, with -> without the fast paths: 1,000 int cases 0.07 -> 0.09 s; 3,000 int 0.34 -> 0.63 s; 10,000 int 3.23 -> 6.27 s; 1,000 string 0.04 -> 0.07 s; 3,000 string 0.17 -> 0.52 s; 10,000 string 1.55 -> 5.38 s; 1,000 polymorphic variant 0.23 -> 0.25 s; 10,000 polymorphic variant 35.47 -> 36.45 s Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JALXUyHhkDqzcB3WuW9Svo Signed-off-by: Christoph Knittel <ck@cca.io>
#8868 made exhaustiveness checking and match compilation find constants by hashing, because an expanded char range turned one pattern into thousands of distinct constants. Char ranges are now one pattern each, so the hash tables in `Parmatch.filter_all`, `Parmatch.get_mins_pats` and `Matching.divide`, and the `const_key` that kept their keys consistent with char/int equality, are removed. A dead `char` check in comparison specialization goes too: `char` expands to `int` before it. Review: How much slower do large switches compile? A switch with 1,000 int or string cases compiles 0.02-0.03 s slower; one with 10,000 int cases takes 6.3 s instead of 3.2 s. Polymorphic variants are unaffected: 10,000 cases take about 35 s either way, as other quadratic steps dominate. Review: Does the output change? No. All committed snapshots are byte-identical, and so is the JS of 1,500 fuzzer programs and the benchmark switches. Area: typechecker lambda Output: unchanged Formatting: unchanged Formats: none PPX: unchanged Editor: unchanged Tests: none, because the output is unchanged and existing tests cover the behaviour Measured: best of 3 bsc wall time, with -> without the fast paths: 1,000 int cases 0.07 -> 0.09 s; 3,000 int 0.34 -> 0.63 s; 10,000 int 3.23 -> 6.27 s; 1,000 string 0.04 -> 0.07 s; 3,000 string 0.17 -> 0.52 s; 10,000 string 1.55 -> 5.38 s; 1,000 polymorphic variant 0.23 -> 0.25 s; 10,000 polymorphic variant 35.47 -> 36.45 s Claude-Session: https://claude.ai/code/session_01JALXUyHhkDqzcB3WuW9Svo Signed-off-by: Christoph Knittel <ck@cca.io> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Why:
A character range pattern is expanded into one or-pattern alternative
per character, and typing, the exhaustiveness and unused-case checks,
default matrices and switch compilation each took time quadratic in
the number of alternatives: a range over the CJK block ran out of
memory (#8866).
Matching modified (character range patterns):
Example:
let f = c =>
switch c {
| '\u{100}' .. '\u{1100}' => 1
| _ => 0
}
Before (lambda):
(let
(f/1001 =
(function c/1002
(catch
(catch
(if (|| (>[int] c/1002 4352) (<[int] c/1002 256)) (exit 2)
(exit 1))
with (2) 0)
with (1) 1)))
(makeblock module/exports f/1001))
After (lambda):
(let
(f/1001 =
(function c/1002
(catch
(catch
(if (|| (>[int] c/1002 4352) (<[int] c/1002 256)) (exit 2)
(exit 1))
with (2) 0)
with (1) 1)))
(makeblock module/exports f/1001))
Before (text): `/usr/bin/time -l bsc` on the example, three runs
2.22 real 2.03 user 0.18 sys
1273511936 maximum resident set size
2.20 real 2.02 user 0.16 sys
1271463936 maximum resident set size
2.21 real 2.03 user 0.17 sys
1273659392 maximum resident set size
After (text): `/usr/bin/time -l bsc` on the example, three runs
0.02 real 0.02 user 0.00 sys
25395200 maximum resident set size
0.02 real 0.02 user 0.00 sys
25624576 maximum resident set size
0.03 real 0.02 user 0.00 sys
25591808 maximum resident set size
Review: Where did the time go? Typing copied the right subtree of each
nested or-pattern to rename variables even when there are none; the
checks searched the groups linearly for each row and compared all
pairs of rows for the default matrix minima; switch compilation found
each key's cell with `List.find`. Groups, minima and int, char and
string cells are now found by hashing, and the copy is skipped when
nothing is renamed.
Review: Does the generated code change? No: #8868 reports byte-identical
JavaScript and warnings across `tests/tests/src`, the `super_errors`
fixtures and the runtime.
Re-applies #8868 by @cknitt for illustration.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Cristiano Calcagno <cristianoc@users.noreply.github.com>
Fixes #8866.
A character range pattern is expanded into one or-pattern alternative per character. Three steps were quadratic in the number of alternatives:
Typecore,Ppat_or): each level of the nested or-pattern ranalpha_patover the entire right subtree, copying it even when there are no variables to rename. This caused the memory blowup. Whenalpha_envis empty the copy is now skipped.enter_orpat_variableshas already checked that both sides bind the same (here: no) variables, so it would be a no-op.Parmatch.filter_all): every row did a linear search for its group. For int, char and string constants and for polymorphic variants,simple_matchis equality, so their groups are now looked up by hashing. Group order and row order stay the same.Parmatch.get_mins le_pats, used byMatching.as_matrix,check_unused,check_partial): compared all pairs of rows. The newget_mins_patsonly compares a row starting with an int/char/string constant with rows starting with the same constant or with a different kind of pattern. No other row can cover it, so the result is identical.Matching.dividefor constants): the cell for each key was found withList.find; it now uses a hash table for int/char/string keys. Floats and bigints keep the linear search, since they compare semantically (1.0=1.).The hash tables are created lazily, because most calls are on small matrices.
'\u{D7FE}' .. '\u{E001}'(~2k chars)'\u{100}' .. '\u{1100}'(~4k chars)'\u{4E00}' .. '\u{9FFF}'(CJK, ~21k chars)'\u{0}' .. '\u{FFFF}'(65k chars)Generated JavaScript and warnings are byte-identical to master across
tests/tests/src, thesuper_errorsfixtures, the runtime, and a stress file. That file has overlapping ranges, duplicate int/string/float/bigint/polyvariant cases, tuples with mixed heads, guards, constructors and arrays, and exercises warnings 8, 11 and 12. On ordinary code (tests/tests + Belt), CPU time is unchanged within noise and allocation is up about 0.5%.Big int/string switches are not fully linear yet. The remaining cost is
check_unusedchecking each row against the previous ones.New test:
tests/tests/src/char_range_test.rescovers the CJK block range and several adjacent ranges, checking boundaries at runtime. Before this change the compiler ran out of memory on it (killed after 39 s at 15 GB).🤖 Generated with Claude Code
https://claude.ai/code/session_01KDBKAdTcAxA3HsjtVESRCr